-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[VM] rename vm catchable and uncatchable exceptions #3538
base: master
Are you sure you want to change the base?
Conversation
@roman-khimov @shargon please take a look, this is what i want to do with the exception currently. It just make it clear what the exception are and enfoce an exception message. If you think it is good to go, i will finish this pr. I think nothing will be influenced. |
Well, need to udpate UT still~~~ UTs are affected since exception type are different |
@@ -59,7 +59,7 @@ public class ExecutionEngine : IDisposable | |||
/// <summary> | |||
/// The VM object representing the uncaught exception. | |||
/// </summary> | |||
public StackItem? UncaughtException { get; internal set; } | |||
public StackItem? UncaughtVMCatchableException { get; internal set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we don't need to rename this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a problem, if you think this pr is ok, will revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you want to throw only VMXXExceptions, ins't it? maybe we should have and interface for valid exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like IVMException
, And catcheable and UnCatcheable
if (string.IsNullOrEmpty(message)) | ||
throw new ArgumentException("Message cannot be null or empty.", nameof(message)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to avoid this, this error will be throw during runtime, is not a warning for us
if (string.IsNullOrEmpty(message)) | ||
throw new ArgumentException("Message cannot be null or empty.", nameof(message)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @Jim8y
Description
This pr tries to improve the VM exception type and inforamtion, making it easier to understand the VM exception.
Fixes # #3536
Type of change
Test Configuration:
Checklist: